Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[iOS] fix: incorrect ScrollView offset on update #30647

Closed
wants to merge 2 commits into from
Closed

[iOS] fix: incorrect ScrollView offset on update #30647

wants to merge 2 commits into from

Conversation

rnike
Copy link
Contributor

@rnike rnike commented Dec 26, 2020 •
edited
Loading

Summary

This pull request is to fix #30258.

After an investigation, I found out the scroll offset of the list seems to be calculated incorrectly due to a workaround in the source code.

Instead of fixing the calculateOffsetForContentSize, I chose to remove it, the reason why I do so is because this workaround is for fixing the offset when contentSize is set manually, but according to the source code, there is no interface for a react-native user to set the contentSize of ScrollView, it is just set to GCSizeZero and will never be changed (ref).

Also I changed the function name from updateContentOffsetIfNeeded to updateContentSizeIfNeeded according what the function is actually doing.

Changelog

[iOS] [Fixed] - Incorrect ScrollView offset on update

Test Plan

  1. Create a fresh new project named testapp using npx create-react-native-app
  2. Apply example code to testapp from https://snack.expo.io/@lokomass/flatlist
  3. Run testapp on iOS emulator and reproduce the bug
  4. Make changes to files in testapp/node_modules/react-native/
  5. Rebuild testapp and run on iOS emulator again, the bug is no more exist
  6. Apply changes from step 4 to react-native, make a pull request.

Screenshots

Before: The scroll offset is incorrect after children of FlatList has changed

Before.mov

After: No more incorrect scroll offset if children of FlatList has changed

After.mov

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 26, 2020
@react-native-bot react-native-bot added Bug Platform: iOS iOS applications. labels Dec 26, 2020
@analysis-bot
Copy link

analysis-bot commented Dec 26, 2020 •
edited
Loading

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 89beefa

facebook-github-bot
facebook-github-bot reviewed Dec 30, 2020
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shergin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@shergin
Copy link
Contributor

shergin commented Dec 30, 2020

Love this PR, I hope this works out!

The contentSize property is declared as assignable (@property (nonatomic, assign) CGSize contentSize;) but the custom setter is missing. We need to fix this first.

shergin
shergin suggested changes Dec 30, 2020
View reviewed changes
Copy link
Contributor

@shergin shergin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my last comment.

@rnike
Copy link
Contributor Author

rnike commented Jan 2, 2021 •
edited
Loading

@shergin I'm not sure what you mean by fixing the contentSize property, I am just removing it as well, because the contentContainerStyle prop is already there to let us manually set the content size, seems that contentSize property is no longer needed.

Please let me know if I am on the right or wrong direction of doing this.

@rnike
Copy link
Contributor Author

rnike commented Jan 2, 2021

Have manually checked on react-native v0.63.4, it works fine.

Wanted to try on v0.64.0-rc.2 but facing a build fail issue:
rn64_build_fail

Cannot check if it works on RN64, I assume that it works as same as v0.63.4.

@rnike rnike requested a review from shergin January 2, 2021 15:54
facebook-github-bot
facebook-github-bot reviewed Jan 4, 2021
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shergin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@shergin merged this pull request in a4526bc.

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Jan 5, 2021
@lokomass
Copy link

lokomass commented Jan 8, 2021

Amazing thanks !!

@lokomass
Copy link

Sorry for this, but I have always this issue with last RN version, is it normal ?

@FERARMAO
Copy link

FERARMAO commented Jul 8, 2021

Just add this prop --> automaticallyAdjustContentInsets={false}

@grahammendick grahammendick mentioned this pull request Sep 1, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@shergin shergin Awaiting requested review from shergin

Assignees
No one assigned
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Platform: iOS iOS applications.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

FlatList remove last item and scroll/height position
7 participants
@rnike @analysis-bot @shergin @facebook-github-bot @lokomass @FERARMAO @react-native-bot